test(http2): cover session.socket after the session is destroyed by robobun · Pull Request #42255 · oven-sh/bun

robobun · 2026-09-11T00:58:41Z

Problem

session.socket must be undefined after an Http2Session is destroyed (Node docs). http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432 (a07354a) fixed both getters, at src/js/node/http2.ts:4554 and :5553. No test covers that behavior.
Before http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432, a session that had handed out its socket proxy returned that proxy forever. After destroy(), session.socket instanceof net.Socket threw ERR_HTTP2_SOCKET_UNBOUND.

Fix

Add test/js/node/http2/node-http2-session-socket.test.ts with one test. There is no change under src/.
The test reads session.socket on the client session and on the server session while the socket is attached. It destroys the client and waits for 'close' on both sessions. Then session.socket is undefined on both sides. A reference taken before destroy() still throws ERR_HTTP2_SOCKET_UNBOUND.
The file uses node:test and node:assert, like node-http2-client-close.test.ts. The same file passes under node --test (v26.3.0) and bun bd test.
Verified: bun bd test test/js/node/http2/node-http2-session-socket.test.ts passes in 25 of 25 runs. With both getters put back to their form before http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432, the test fails for both sessions (after: 'object', afterIsSocket: 'ERR_HTTP2_SOCKET_UNBOUND').

Background

An Http2Session owns a net.Socket or a tls.TLSSocket. session.socket returns a Proxy around the session, not the socket. The Proxy blocks methods such as write and destroy, because a raw write corrupts the HTTP/2 framing.
Each session caches that Proxy in #socket_proxy. destroy() clears the real socket (this[bunHTTP2Socket]). From then on every trap of the cached Proxy throws ERR_HTTP2_SOCKET_UNBOUND. That includes getPrototypeOf, which instanceof calls.
Teardown code often guards with if (session.socket && !session.socket.destroyed). That guard needs undefined, not the stale Proxy.

Notes

The test comes from node:http2: return undefined from session.socket after destroy() #33791. That PR proposed the same getter change and is now closed, because http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432 landed it first. The test there covered the client session only. This version also covers the server session, because http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432 changed both getters.
The first version of this PR added the test to node-http2.test.js. It now has its own file. A run of that whole file with a debug build has timing sensitive tests: on a machine under load, the http2 DATA payload survives its ArrayBuffer being detached/resized by transport JS mid-send block reaches its 5000 ms timeouts (9 of 9 with 3 CPUs), and http2 server with minimal maxSessionMemory handles multiple requests timed out once on macOS aarch64 in build 114070 before it passed on the retry. The new test passed in each of those runs.
The test waits for the server 'session' event before it destroys the client. So the server always has a session to close, and the test does not depend on the accept order.

Output with the getters of main:

$ node --test test/js/node/http2/node-http2-session-socket.test.ts
✔ session.socket is undefined after the session is destroyed (13.511606ms)
ℹ tests 1
ℹ pass 1
ℹ fail 0

$ bun bd test test/js/node/http2/node-http2-session-socket.test.ts
(pass) session.socket is undefined after the session is destroyed [695.49ms]
 1 pass
 0 fail

Output with the getters put back to their form before http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432:

  client: {
+     after: 'object',
+     afterIsSocket: 'ERR_HTTP2_SOCKET_UNBOUND',
-     after: 'undefined',
-     afterIsSocket: false,
      before: 'object',
      beforeIsSocket: 'ERR_HTTP2_SOCKET_UNBOUND'
  },
  server: { (the same four lines) }

test/js/node/test/parallel/test-http2-unbound-socket-proxy.js already covers the captured reference on the client. It does not read session.socket again after the close.

[auto-merge] gate passed · iteration 1 · 1 files touched

passes on PR (with fix)

Test-only change.

Debug/ASAN (expected pass):
$ bun bd test 'test/js/node/http2/node-http2-session-socket.test.ts'
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "test/js/node/http2/node-http2-session-socket.test.ts"
bun test v1.4.3 (4ff919377)

test/js/node/http2/node-http2-session-socket.test.ts:
(pass) session.socket is undefined after the session is destroyed [726.66ms]

 1 pass
 0 fail
Ran 1 test across 1 file. [3.51s]
Exit: 0

diff hotspot

.../node/http2/node-http2-session-socket.test.ts   | 75 ++++++++++++++++++++++
 1 file changed, 75 insertions(+)

gate history · 1 passed · 1 rejected · iteration 1

evidence per changed file

file                                                  reads  edits  tests
test/js/node/http2/node-http2-session-socket.test.ts      0      1      7

#34432 made the socket getters of ServerHttp2Session and ClientHttp2Session return undefined once the session has detached from its socket, also when the session had already handed out its cached proxy. No test covers that behavior. The test reads session.socket on both sides while the socket is attached, destroys the client, and waits for both sessions to close. Then session.socket is undefined on both sides, and a reference taken before destroy() still throws ERR_HTTP2_SOCKET_UNBOUND, as in Node.

coderabbitai · 2026-09-11T01:00:05Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: f6f9baaf-9e14-4b83-b4f9-ba50a42d960e

📥 Commits

Reviewing files that changed from the base of the PR and between 852ed6e and d94abfe.

📒 Files selected for processing (1)

test/js/node/http2/node-http2-session-socket.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.

Walkthrough

The change adds an HTTP/2 regression test for client and server session socket behavior after destruction. It verifies that current socket access becomes undefined and cached socket proxies throw ERR_HTTP2_SOCKET_UNBOUND, including through instanceof.

Changes

HTTP/2 socket lifecycle

Layer / File(s)	Summary
Session socket destruction regression `test/js/node/http2/node-http2-session-socket.test.ts`	The test captures client and server socket proxies, destroys the client session, waits for both sessions to close, and checks post-destruction socket access and errors.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to d94ab

The added regression coverage does not introduce a current merge-blocking risk.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the test change and the HTTP/2 behavior it covers. It is concise and directly related to the pull request.
Description check	✅ Passed	The description explains the problem, the test changes, expected behavior, verification commands, and regression coverage. It does not use the exact template headings, but it provides the required inf…

_{Comment @coderabbitai help to get the list of available commands.}

robobun · 2026-09-11T01:02:15Z

Status: ready for review. This is a test-only change with no diff under src/.

How to check it:

Run bun bd test test/js/node/http2/node-http2-session-socket.test.ts. It passes on main, 25 of 25 runs with the debug ASAN build.
Run node --test test/js/node/http2/node-http2-session-socket.test.ts. It passes on Node v26.3.0.
Put the two get socket() getters in src/js/node/http2.ts back to their form before http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432 and run it again. The test fails for the client session and for the server session (after: 'object', afterIsSocket: 'ERR_HTTP2_SOCKET_UNBOUND').

CI: in build 114097 the new test passes on every lane (Linux x64 and aarch64, the ASAN lane, Windows x64 and aarch64, macOS x64 and aarch64). The only red job is test/js/bun/http/serve-pending-promise-abort-leak.test.ts on debian 13 x64-asan. That test also fails on main, and #42190 is open for it. This PR does not touch it.

The test replaces #33791, which is closed because #34432 landed the same getter change.

claude

LGTM — test-only addition, well-scoped and follows the file's conventions.

What was reviewed:

Test hygiene: port: 0 on 127.0.0.1, Promise.withResolvers() for event sync (no sleeps), error events wired to reject, try/finally cleanup registered before assertions.
Assertion strength: checks both client and server sessions, asserts the exact ERR_HTTP2_SOCKET_UNBOUND code on the pre-destroy proxy reference, and toBeUndefined() on the post-destroy getter.
Confirmed net is already imported at the top of the file; no CODEOWNERS coverage for this path.

Extended reasoning...

Overview

This PR adds a single 44-line test case to test/js/node/http2/node-http2.test.js covering the Node-documented behavior that Http2Session#socket returns undefined after the session is destroyed. It exercises both client and server sessions: it captures the socket Proxy before destroy, destroys the client, awaits close on both sides, then asserts the getter now returns undefined, instanceof net.Socket is false, and the previously-captured Proxy still throws ERR_HTTP2_SOCKET_UNBOUND on instanceof. No production code is touched — the fix landed in #34432 and this backfills coverage.

Security risks

None. The test spins up an in-process plaintext HTTP/2 server on 127.0.0.1:0 and connects to it locally. There is no external network access, no credential handling, no untrusted-input parsing, and no changes to runtime code.

Level of scrutiny

Low. This is a small, self-contained test addition to an existing compatibility test file, following the exact patterns already present in that file (http2.createServer(), Promise.withResolvers(), try/finally with server.close()). I checked it against the test-authoring rules in CLAUDE.md and REVIEW.md: it uses port 0 on 127.0.0.1, awaits events rather than time, wires error handlers to reject the awaited promises, registers cleanup before assertions, and asserts the specific error code rather than a bare toThrow(). The net import it relies on already exists at the top of the file.

Other factors

The PR description states the test fails when the getters are reverted to their pre-#34432 form and passes 25/25 on the debug build, and that Node v26.3.0 produces identical results — consistent with the Node-compat testing bar. The changed path is not covered by CODEOWNERS. There are no prior reviews or outstanding objections on the timeline, and the bug hunt exited on dry_streak with no findings.

robobun · 2026-09-11T01:11:17Z

^{Updated 8:14 PM PT - Sep 10th, 2026}

❌ @robobun, your commit d94abfe has 1 failures in Build #114097 (All Failures):

test/js/bun/http/serve-pending-promise-abort-leak.test.ts - code 1 on 🐧 13 x64-asan

🧪 To try this PR locally:

bunx bun-pr 42255

That installs a local version of the PR into your bun-42255 executable, so you can run:

bun-42255 --bun

test error

The test now uses node:test and node:assert, like node-http2-client-close.test.ts, so the same file runs under `bun test` and under `node --test`. It compares one probe object for the client session and one for the server session. In its own file the test no longer shares a run with the timing sensitive tests of node-http2.test.js.

robobun · 2026-09-11T02:10:27Z

@cirospaciari about the test error: it did not come from the new test.

The failed robobun/evidence check ran all of node-http2.test.js once with a debug build, with no retry. That file has timing sensitive tests. With the CPU count cut to 3, the same exit code 1 reproduces: all 9 tests of the http2 DATA payload survives its ArrayBuffer being detached/resized by transport JS mid-send block reach their 5000 ms timeout. The new test passed in that run, and in 4 of 4 full runs on an idle machine.
In build 114070 node-http2.test.js passed on every lane that has finished (ASAN, Windows, macOS aarch64, Linux). On macOS aarch64 it needed its retry: http2 server with minimal maxSessionMemory handles multiple requests timed out at 15000 ms on the first attempt. The new test passed in both attempts.
The only red job in that build is test/js/bun/http/serve-pending-promise-abort-leak.test.ts on debian 13 x64-asan. It also fails on main, and test: tolerate one conservative-GC survivor in the aborted body stream check #42190 is open for it.

d94abfe moves the new test to its own file, test/js/node/http2/node-http2-session-socket.test.ts, so that it no longer shares a run with those tests. The file uses node:test and node:assert, like node-http2-client-close.test.ts, so it also passes under node --test (v26.3.0). It checks the same things as before.

claude

LGTM — test-only change, and the move to a standalone node:test file matches the sibling node-http2-continuation.test.ts / node-http2-upgrade.test.mts pattern.

What was reviewed:

Event sequencing: once() rejects on 'error' for listen/connect; server 'session' is awaited before destroy; both 'close' events are awaited — no sleeps or timing races.
The server session's no-op 'error' handler is correct here (client destroy() is expected to error the peer); it can't mask a real failure because the deepStrictEqual runs after both closes.
Cleanup is in try/finally registered before assertions; port: 0 on 127.0.0.1; single deep-equal on a combined object with the exact ERR_HTTP2_SOCKET_UNBOUND code.

Extended reasoning...

Overview

This PR adds a single 75-line test file, test/js/node/http2/node-http2-session-socket.test.ts, covering Http2Session#socket behavior after destroy(). It creates an h2 server on 127.0.0.1:0, connects a client, captures session.socket on both sides while attached (forcing the Proxy cache), destroys the client, awaits 'close' on both sessions, then asserts via one deepStrictEqual that session.socket is now undefined on both sides while the pre-captured Proxy still throws ERR_HTTP2_SOCKET_UNBOUND on instanceof. There are no changes under src/. Since the prior review, two commits landed; the final one moved the test into its own file so it can run identically under node --test, matching two existing siblings in the same directory that already use node:test.

Security risks

None. Test-only, no network egress beyond loopback, no credentials, no changes to runtime code paths.

Level of scrutiny

Low-to-moderate: a small, self-contained test addition. The main things to check for a test PR here are flakiness, hermeticity, and whether the assertion can actually fail for the right reason. All events are awaited (once from node:events rejects on 'error', so listen/connect failures surface); the server 'session' promise is awaited before destroy so the accept order can't race; both 'close' events are awaited before probing. The server session's swallowed 'error' is deliberate — destroying the client is expected to error the peer — and cannot hide a hang because the subsequent 'close' await and the final deepStrictEqual still gate the test. Cleanup is in try/finally registered before the assertion, port: 0, IPv4 loopback, no per-test timeout.

Other factors

The "add to the existing file" default is satisfied by directory precedent: node-http2-continuation.test.ts and node-http2-upgrade.test.mts already use node:test in standalone files here, and the file header documents the dual-runner intent. The assertion checks the exact ERR_* code and uses a single combined deep-equal, per repo convention. The only third-party review (cirospaciari) is DISMISSED, so there are no outstanding objections. Exit reason was dry_streak.

github-actions Bot added the claude label Sep 11, 2026

robobun mentioned this pull request Sep 11, 2026

node:http2: return undefined from session.socket after destroy() #33791

Closed

claude Bot reviewed Sep 11, 2026

View reviewed changes

ci: retrigger

852ed6e

cirospaciari previously approved these changes Sep 11, 2026

View reviewed changes

cirospaciari enabled auto-merge (squash) September 11, 2026 01:21

cirospaciari disabled auto-merge September 11, 2026 01:21

claude Bot reviewed Sep 11, 2026

View reviewed changes

robobun requested a review from cirospaciari September 11, 2026 03:12

Jarred-Sumner mentioned this pull request Oct 22, 2026

http: consolidate the open fetch, Bun.serve, node:http, node:http2 and HTTP/3 pull requests #46528

Open

Navigation Menu

Repository navigation

test(http2): cover session.socket after the session is destroyed - #42255

test(http2): cover session.socket after the session is destroyed#42255
robobun wants to merge 3 commits into
mainfrom
robobun/86e90bb9/http2-session-socket-destroy-test

robobun commented Sep 11, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Sep 11, 2026 •

edited

Loading

Uh oh!

robobun commented Sep 11, 2026 •

edited

Loading

Uh oh!

claude Bot left a comment

Uh oh!

robobun commented Sep 11, 2026 •

edited

Loading

Uh oh!

robobun commented Sep 11, 2026

Uh oh!

claude Bot left a comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

robobun commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Background

Uh oh!

coderabbitai Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

robobun commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robobun commented Sep 11, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Sep 11, 2026 •

edited

Loading

coderabbitai Bot commented Sep 11, 2026 •

edited

Loading

robobun commented Sep 11, 2026 •

edited

Loading

robobun commented Sep 11, 2026 •

edited

Loading